zend_atoi/zend_atol cleanup by sgolemon · Pull Request #4132 · php/php-src

sgolemon · 2019-05-08T18:54:36Z

zend_ato[il]() don't just do number parsing.
They also check for a 'K', 'M', or 'G' at the end of the string,
and multiply the parsed value out accordingly.

Unfortunately, they ignore any other non-numerics between the
numeric component and the last character in the string.
This means that numbers such as the following are both valid
and non-intuitive in their final output.

"123KMG" is interpreted as "123G" -> 132070244352
"123G " is interpreted as "123 " -> 123
"123GB" is interpreted as "123B" -> 123
"123 I like tacos." is also interpreted as "123." -> 123

This diff primarily adds warnings for these cases when the output
would be a potentially unexpected, and unhelpful value.

Additionally, several places in php-src use these functions
despite not actually wanting their KMG behavior such as
session.upload_progress.freq which will happily parse "1 banana"
as a valid value.

For these settings, I've switched them to ZEND_STRTOL which preserves
their existing /intended/ behavior.

It won't respect KMG suffixes, but they never really wanted that logic anyway.
It will ignore non-numeric suffixes so as to not introduce new failures.

We should probably reexamine that second bullet point separately.

Lastly, with these changes, zend_atoi() is no longer used in php-src,
but I left it as a valid API for 3rd party extensions.
Note that I did make it a proxy to zend_atol() since deferring the
truncation till the end is effectively the same as truncation during
multiplication, but this avoid code duplication.

I think we should consider removing zend_atoi() entirely (perhaps in 8.0?)
and rename zend_atol() to something reflecting it's KMG specific behavior.

nikic · 2019-05-09T14:12:40Z

Commit these fixes separately?

Happy to split 'em up, sure.

nikic · 2019-05-09T14:16:02Z

Maybe use %.*s here with explicitly passed length to avoid those string copies? Can also use %c instead of the suffixstr.

Ah, right. %.*s was the format I was trying to recall. Thanks.

On the suffixstr one, I don't want an empty space introduced when it's \0.

nikic · 2019-05-09T14:19:14Z

What happens if you pass just M for example?

Some tests would be nice :) Otherwise this looks good.

sgolemon · 2019-05-09T15:23:57Z

What happens if you pass just M for example?

The same thing that happened before. ZEND_STRTOL() gives us 0 which is multiplied by 1024 * 1024, yielding 0. We should probably add a warning for that as well.

Some tests would be nice :) Otherwise this looks good.

Yeah, in my email to internals I mentioned that tests are coming, I just wanted to get the conversation going first.

cmb69 · 2019-05-09T17:58:48Z

Shouldn't the base argument to ZEND_STRTOL be 10 (elsewhere as well)?

For these changes I chose not to make any unnecessary changes.
If we set "10" here, then ONLY base 10 strings will be respected.
That's probably fine for most of these sites, but I don't have a pressing need to say "You can't use octal/hex".

Ah, indeed. I thought zend_atoi() would only accept base-10 numbers (the misnomer is rather confusing).

Llbe · 2019-05-11T08:44:58Z

Will this make INI memory_limit more strict?

E.g. var_dump(ini_set('memory_limit', 'foo'), ini_get('memory_limit')); → "foo" is accepted as a value.

sgolemon · 2019-05-12T23:38:26Z

Will this make INI memory_limit more strict?

Yes and no. It'll warn on bad values, but it'll still accept them.

E.g. var_dump(ini_set('memory_limit', 'foo'), ini_get('memory_limit')); → foo is accepted as a value.

It's a value, but it's a terrible one. :)
You're setting your memory limit to 0 there.

Specifically, you'd get:
Warning: Invalid numeric string 'foo', no valid leading digits, interpreting as '0'

javiereguiluz · 2019-05-13T06:31:57Z

Minor typo? timout -> timeout

zend_ato[il]() don't *just* do number parsing. They also check for a 'K', 'M', or 'G' at the end of the string, and multiply the parsed value out accordingly. None of these use cases actually call for this behavior, so invoke ZEND_STRTOL() separately.

zend_ato[il]() don't just do number parsing. They also check for a 'K', 'M', or 'G' at the end of the string, and multiply the parsed value out accordingly. Unfortunately, they ignore any other non-numerics between the numeric component and the last character in the string. This means that numbers such as the following are both valid and non-intuitive in their final output. * "123KMG" is interpreted as "123G" -> 132070244352 * "123G " is interpreted as "123 " -> 123 * "123GB" is interpreted as "123B" -> 123 * "123 I like tacos." is also interpreted as "123." -> 123 This diff primarily adds warnings for these cases when the output would be a potentially unexpected, and unhelpful value. Additionally, with these changes, zend_atoi() is no longer used in php-src, but I left it as a valid API for 3rd party extensions. Note that I did make it a proxy to zend_atol() since deferring the truncation till the end is effectively the same as truncation during multiplication, but this avoid code duplication. I think we should consider removing zend_atoi() entirely (perhaps in 8.0?) and rename zend_atol() to something reflecting it's KMG specific behavior.

arnaud-lb · 2021-12-11T17:08:59Z

I like this change

@sgolemon do you mind if I take over ?

zend_atol() parses integers with size suffixes (like "128M"). Here zend_atol() is replaced with ZEND_ATOL() when the intent was not to parse a size or quantity. This builds on 989205e and phpGH-4132.

cmb69 · 2022-01-16T16:08:08Z

+			break;
+		default:
+			/* Unknown suffix */
+			zend_error(E_WARNING, "Invalid numeric string '%.*s', interpreting as '%.*s'",


Not sure about the warning message ("Invalid numeric string"). Should that be more like "invalid suffix"? Same a few lines below.

ramsey · 2022-05-27T16:42:37Z

There are several PRs floating around right now that deal with this area:

I suspect that #7788 supersedes this PR and #7951 might even negate the need for it.

Thoughts on closing this one?

cmb69 · 2022-05-27T16:59:36Z

Yeah, this PR should already have been closed.

nikic reviewed May 9, 2019

View reviewed changes

sgolemon force-pushed the zend_atol branch from 51a2ce5 to 5d0bcd8 Compare May 9, 2019 16:35

cmb69 reviewed May 9, 2019

View reviewed changes

javiereguiluz reviewed May 13, 2019

View reviewed changes

sgolemon added 2 commits May 15, 2019 12:06

sgolemon force-pushed the zend_atol branch from 5d0bcd8 to 714d76f Compare May 15, 2019 16:08

petk added the Feature label May 16, 2019

arnaud-lb mentioned this pull request Dec 18, 2021

zend_atol() cleanup #7788

Closed

arnaud-lb mentioned this pull request Jan 16, 2022

Deprecate zend_atol() / add zend_ini_parse_quantity() #7951

Merged

cmb69 reviewed Jan 16, 2022

View reviewed changes

dmsnell mentioned this pull request Apr 28, 2022

Add ini_parse_quantity to convert ini directive "byte" shorthand to int #8454

Merged

ramsey added the Category: Engine label May 27, 2022

cmb69 closed this May 27, 2022

Uh oh!

Conversation

sgolemon commented May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic commented May 9, 2019

Uh oh!

sgolemon commented May 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Llbe commented May 11, 2019

Uh oh!

sgolemon commented May 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arnaud-lb commented Dec 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramsey commented May 27, 2022

Uh oh!

cmb69 commented May 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

sgolemon commented May 8, 2019 •

edited

Loading

sgolemon commented May 12, 2019 •

edited

Loading